[TRTLLM-13409][feat] Make a stalled executor worker init self-report - #16973
[TRTLLM-13409][feat] Make a stalled executor worker init self-report#16973JunyiXu-nv wants to merge 2 commits into
Conversation
|
/bot run |
|
PR_Github #62336 [ run ] triggered by Bot. Commit: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughExecutor startup now has configurable stall warnings, per-rank watchdogs, stack-dump logging, and tests for startup handshakes, configuration parsing, watchdog lifecycle, and disarm ordering. ChangesExecutor startup stall diagnostics
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GenerationExecutorProxy
participant worker_init_status_queue
participant worker_main
participant logger
GenerationExecutorProxy->>worker_init_status_queue: poll initialization status
GenerationExecutorProxy->>logger: emit periodic stall report
worker_main->>logger: warn and dump thread stacks
worker_init_status_queue-->>GenerationExecutorProxy: ready or error status
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/_utils.py`:
- Around line 703-715: Update print_all_stacks to annotate its optional log
parameter with a precise Callable type matching the emitted string message, and
annotate its return type as None. Preserve the existing default logger.error
behavior and stack-trace emission.
In `@tensorrt_llm/executor/proxy.py`:
- Around line 688-708: Update _worker_init_stall_report to stop inferring worker
liveness or asserting that no rank has exited from mpi_futures. Use an
authoritative liveness result if one is available; otherwise describe
running/finished local futures only, handle 0/0, and remove or qualify the “not
a worker crash” claim.
In `@tensorrt_llm/executor/utils.py`:
- Around line 323-333: Update float_from_env to reject non-finite parsed values
by validating the float with math.isfinite() before returning it; treat NaN and
infinities like invalid input, log the existing warning, and return default.
In `@tensorrt_llm/executor/worker.py`:
- Around line 437-439: Update the startup readiness flow around
notify_with_retry() and worker_init_done so the completion event is set only
when the ready notification is delivered successfully. When notify_with_retry()
returns False, keep the leader watchdog active by continuing retries or leaving
startup pending rather than unconditionally calling worker_init_done.set().
In `@tests/unittest/executor/test_proxy_worker_startup.py`:
- Line 182: Replace the broad BaseException handlers in both background
thread-runner closures with Exception handlers, including the closure around
_start_executor_workers and the handler at the additional referenced location.
Preserve the existing RuntimeError capture and handling while allowing
KeyboardInterrupt and SystemExit to propagate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f1e7ec9d-062e-4080-b841-35efa3b71fe7
📒 Files selected for processing (5)
tensorrt_llm/_utils.pytensorrt_llm/executor/proxy.pytensorrt_llm/executor/utils.pytensorrt_llm/executor/worker.pytests/unittest/executor/test_proxy_worker_startup.py
| def float_from_env(name: str, default: float) -> float: | ||
| """Read a float-valued environment variable, falling back on bad input.""" | ||
| raw = os.getenv(name) | ||
| if raw is None or not raw.strip(): | ||
| return default | ||
| try: | ||
| return float(raw) | ||
| except ValueError: | ||
| logger.warning( | ||
| f"Ignoring invalid {name}={raw!r}; using default {default}.") | ||
| return default |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python - <<'PY'
import math
for raw in ("nan", "inf", "-inf"):
value = float(raw)
assert not math.isfinite(value)
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- utils.py around float_from_env ---\n'
sed -n '300,360p' tensorrt_llm/executor/utils.py
printf '\n--- search for worker_init_stall_warn_sec ---\n'
rg -n "worker_init_stall_warn_sec|float_from_env\(" tensorrt_llm -S
printf '\n--- relevant worker.py ranges ---\n'
sed -n '1,260p' tensorrt_llm/executor/worker.py
printf '\n--- proxy/timeout usage search ---\n'
rg -n "Event\.wait\(|timeout|stall_warn|watchdog|deadline|period <= 0" tensorrt_llm/executor -SRepository: NVIDIA/TensorRT-LLM
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '300,360p' tensorrt_llm/executor/utils.py
rg -n "worker_init_stall_warn_sec|float_from_env\(" tensorrt_llm -S
sed -n '1,260p' tensorrt_llm/executor/worker.py
rg -n "Event\.wait\(|timeout|stall_warn|watchdog|deadline|period <= 0" tensorrt_llm/executor -SRepository: NVIDIA/TensorRT-LLM
Length of output: 35643
🏁 Script executed:
python3 - <<'PY'
import threading, time, math
ev = threading.Event()
for timeout in [math.nan, math.inf, -math.inf, 0.0, 0.1]:
start = time.monotonic()
try:
result = ev.wait(timeout=timeout)
elapsed = time.monotonic() - start
print(timeout, result, round(elapsed, 6))
except Exception as exc:
print(timeout, type(exc).__name__, exc)
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 288
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '640,685p' tensorrt_llm/executor/proxy.py
python3 - <<'PY'
import math, time
stall_warn_sec_values = [math.nan, math.inf, -math.inf, 0.0, 1.0]
for stall_warn_sec in stall_warn_sec_values:
if stall_warn_sec > 0:
next_warn_time = time.monotonic() + stall_warn_sec
print(stall_warn_sec, "armed", next_warn_time, time.monotonic() >= next_warn_time)
else:
print(stall_warn_sec, "disabled")
PYRepository: NVIDIA/TensorRT-LLM
Length of output: 2532
Reject non-finite stall intervals.
float() accepts nan and inf. nan slips past the <= 0 check and makes the worker watchdog spin immediately; inf makes Event.wait() blow up in the worker and pushes the proxy stall report out to infinity. Reject non-finite values with math.isfinite() and fall back to the default.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/executor/utils.py` around lines 323 - 333, Update float_from_env
to reject non-finite parsed values by validating the float with math.isfinite()
before returning it; treat NaN and infinities like invalid input, log the
existing warning, and return default.
There was a problem hiding this comment.
+1, with =nan the watchdog spins with no sleep, dumping every thread's stack at WARNING in every rank (8 processes on TP=8), while the proxy stays silent; and given this PR's own premise that startup logs are where information gets lost, I'd raise rather than fall back to the default.
There was a problem hiding this comment.
@zhaoyangwang-nvidia, agreed. Falling back turns an explicit but invalid configuration into a noisy, asymmetric failure mode; raising makes the bad value immediately actionable and avoids losing the intended startup diagnostics.
- Make
TRTLLM_WORKER_INIT_STALL_WARN_SECparsing fail fast for malformed or non-finite explicit values. - Preserve the default only when the variable is unset or blank.
- Update the unit tests to cover
nan,inf, and-infraising during configuration parsing.
✏️ Learnings added
Learnt from: zhaoyangwang-nvidia
Repo: NVIDIA/TensorRT-LLM PR: 16973
File: tensorrt_llm/executor/utils.py:323-333
Timestamp: 2026-07-30T08:42:11.520Z
Learning: For executor startup stall diagnostics, an explicitly configured invalid `TRTLLM_WORKER_INIT_STALL_WARN_SEC` value—including `nan` and infinities—must fail fast rather than fall back to the default. Silent fallback can cause every worker watchdog to flood warning stack dumps while the proxy emits no stall report.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- 📌 Commit the changes to this branch
You are interacting with an AI system.
| def start_workers(): | ||
| try: | ||
| proxy._start_executor_workers({}) | ||
| except BaseException as exc: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Narrow the blind except BaseException in the background thread runners.
Both thread-runner closures catch BaseException, which also swallows KeyboardInterrupt/SystemExit. except Exception is enough to capture the RuntimeError raised by _start_executor_workers and is what the guideline calls for.
As per coding guidelines, "Catch specific exceptions instead of using broad or bare exception handling such as except:." Static analysis also flags this (BLE001 at line 182, S110 at 218-219), though those specific rule families aren't in this repo's configured Ruff select set per prior learnings.
🐛 Proposed fix
def start_workers():
try:
proxy._start_executor_workers({})
- except BaseException as exc:
+ except Exception as exc:
result["exception"] = exc def start_workers():
try:
proxy._start_executor_workers({})
- except BaseException: # noqa: BLE001 - the release path, not the subject
+ except Exception: # the release path, not the subject
passAlso applies to: 218-219
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 182-182: Do not catch blind exception: BaseException
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/executor/test_proxy_worker_startup.py` at line 182, Replace
the broad BaseException handlers in both background thread-runner closures with
Exception handlers, including the closure around _start_executor_workers and the
handler at the additional referenced location. Preserve the existing
RuntimeError capture and handling while allowing KeyboardInterrupt and
SystemExit to propagate.
Sources: Coding guidelines, Linters/SAST tools
|
PR_Github #62336 [ run ] completed with state
|
Pipeline #50503 — two failures, and I am not claiming they are unrelatedTwo failing testcases across the 19 stages that uploaded results: Measured ambient rate over the last 55 pipelines:
Why that is not enough to dismiss them. On my other PRs I have been able to argue that the change cannot reach the failing test. I cannot make that argument here. This PR modifies Two candidate mechanisms I considered and could not rule out by inspection:
Against that: the watchdog only logs, Not re-running yet. Once the scaffolding blocker clears I will re-run, and treat a repeat of either failure as evidence against this PR rather than as noise. |
|
#16978 has merged, so the collection error no longer fails DGX_H100 stages. Re-running now that the result will actually be informative. To restate the standard I set on this PR: the two failures from #50503 — So this run is a test of the change, not a retry for a green light. If either failure repeats, I will treat that as evidence against this PR and investigate the watchdog thread and the startup-path changes rather than look for a third explanation. |
|
/bot run |
|
#16978 has merged, so the To restate the standard I set on this PR: the two failures from #50503 — So this run is a test of the change, not a retry hoping for a green light. If either failure repeats, I will treat that as evidence against this PR — and investigate the per-rank watchdog thread and the startup-path changes — rather than reach for a third flake explanation. |
|
PR_Github #62396 [ run ] triggered by Bot. Commit: |
|
PR_Github #62396 [ run ] completed with state
|
|
/bot run |
|
PR_Github #62672 [ run ] triggered by Bot. Commit: |
|
PR_Github #62672 [ run ] completed with state
|
| def float_from_env(name: str, default: float) -> float: | ||
| """Read a float-valued environment variable, falling back on bad input.""" | ||
| raw = os.getenv(name) | ||
| if raw is None or not raw.strip(): | ||
| return default | ||
| try: | ||
| return float(raw) | ||
| except ValueError: | ||
| logger.warning( | ||
| f"Ignoring invalid {name}={raw!r}; using default {default}.") | ||
| return default |
There was a problem hiding this comment.
+1, with =nan the watchdog spins with no sleep, dumping every thread's stack at WARNING in every rank (8 processes on TP=8), while the proxy stays silent; and given this PR's own premise that startup logs are where information gets lost, I'd raise rather than fall back to the default.
4aeb0c3 to
9ccf505
Compare
Both findings fixed —
|
| input | threading.Event().wait(x) |
|---|---|
nan |
returns False in ~10 µs — so while not init_done.wait(period) becomes a hot spin, re-entering print_all_stacks(log=logger.warning) as fast as the interpreter allows, on every rank |
-inf |
same hot spin |
inf |
raises OverflowError: timestamp out of range for platform time_t — kills the watchdog thread silently, the opposite failure and equally bad for a diagnostic |
So your read was exactly right, and inf fails the other way.
float_from_env now raises on both non-finite and unparsable input; only unset/blank falls back. I took your point about raising rather than defaulting and applied it to the pre-existing ValueError path too — the variable is TRTLLM_-prefixed, only ever set deliberately, and read at precisely the moment this PR argues information gets lost. The proxy also reads the knob at the top of _start_executor_workers, before mpi_session.submit(), so a bad value now fails while there are still no ranks to orphan.
2. The 0/0 report — the wording is gone, not reworded.
You called out that the suggested wording still printed 0/0, so the report is now conditional and that string can no longer be emitted. With no futures it reads:
whether a rank has exited cannot be told from here: this MPI session hands back no worker futures, so the proxy has no liveness signal during startup and this report can neither confirm nor rule out a crashed rank. The session's
check_worker_error()channel is authoritative for that
The docstring cites RemoteMpiCommSessionClient.submit() returning [] and points at the pre_shutdown() comment you referenced as the in-tree precedent.
Two notes for the thread. check_worker_error() is deliberately not called from the report — reading it consumes the death notice _check_remote_worker_death() acts on. And it is worse than "under-informed": _error_monitor_thread starts at proxy.py:217, after _start_executor_workers returns at :205, so during the init wait the proxy has zero liveness visibility on that session type, not merely a stale view.
Verification. 27 passed, 15 warnings in 0.41s, exit 0. Three mutations, each reverted after:
| Mutation | Result |
|---|---|
drop the math.isfinite guard |
8 failed, 19 passed — killed by the non-finite and never-arms/never-reaches-loop tests |
restore the silent ValueError fallback |
1 failed, 26 passed |
restore the unconditional {running}/{total} … so no rank has exited |
2 failed, 25 passed |
One thing worth stating rather than hiding: the first draft of one new test hung under mutation instead of failing (killed at 120 s) — with nan, nan > 0 is False so the startup loop never armed its deadline. A hang is not a kill, so the test was rewritten to use a pre-failed future and now fails cleanly. The table above is from the rewritten version.
Not done: no hardware run. Both fixes are validation and string logic on paths the existing fakes already drive, and the empty-mpi_futures branch is established statically from mpi_session.py:610. Happy to run it under TLLM_SPAWN_PROXY_PROCESS=1 if you would rather see it confirmed dynamically.
|
/bot run |
|
PR_Github #62990 [ run ] triggered by Bot. Commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tensorrt_llm/executor/utils.py (1)
326-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd complete Google-style docstrings for the public functions.
float_from_env()does not document its arguments, return value, orValueError.worker_init_stall_warn_sec()has no docstring. AddArgs:,Returns:, andRaises:sections where applicable.As per coding guidelines: “Prefer docstrings for external interfaces, use Google-style docstrings, document public function arguments.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tensorrt_llm/executor/utils.py` around lines 326 - 360, Expand the docstring for float_from_env with Google-style Args, Returns, and Raises sections covering name, default, the returned float, and invalid non-finite or unparsable values. Add a complete Google-style docstring to worker_init_stall_warn_sec describing its returned watchdog warning interval; preserve the existing behavior and explanatory details.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unittest/executor/test_proxy_worker_startup.py`:
- Around line 15-39: Add tests/unittest/executor/test_proxy_worker_startup.py to
the appropriate CI test list or test database entry so all 20 startup test
functions execute in CI. Do not modify or remove the tests; update only the
existing list configuration used for executor unit-test coverage.
---
Nitpick comments:
In `@tensorrt_llm/executor/utils.py`:
- Around line 326-360: Expand the docstring for float_from_env with Google-style
Args, Returns, and Raises sections covering name, default, the returned float,
and invalid non-finite or unparsable values. Add a complete Google-style
docstring to worker_init_stall_warn_sec describing its returned watchdog warning
interval; preserve the existing behavior and explanatory details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 02561d6b-c3df-41f0-8041-d72d1483c8f2
📒 Files selected for processing (5)
tensorrt_llm/_utils.pytensorrt_llm/executor/proxy.pytensorrt_llm/executor/utils.pytensorrt_llm/executor/worker.pytests/unittest/executor/test_proxy_worker_startup.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tensorrt_llm/_utils.py
- tensorrt_llm/executor/worker.py
- tensorrt_llm/executor/proxy.py
| """Startup handshake between the executor proxy and its MPI workers. | ||
|
|
||
| Every proxy test here drives the real | ||
| ``GenerationExecutorProxy._start_executor_workers``; only the MPI session and | ||
| the init status queue are faked, so no GPU (and no MPI spawn) is needed. The | ||
| worker-side tests drive the real | ||
| ``tensorrt_llm.executor.worker._worker_init_stall_watchdog`` / | ||
| ``_arm_worker_init_stall_watchdog``. | ||
| """ | ||
|
|
||
| import ast | ||
| import pathlib | ||
| import queue | ||
| import sys | ||
| import threading | ||
| import time | ||
| import types | ||
| from concurrent.futures import Future | ||
|
|
||
| import pytest | ||
|
|
||
| from tensorrt_llm._utils import print_all_stacks | ||
| from tensorrt_llm.executor import worker as worker_module | ||
| from tensorrt_llm.executor.proxy import GenerationExecutorProxy | ||
| from tensorrt_llm.executor.utils import WORKER_INIT_STALL_WARN_ENV, worker_init_stall_warn_sec |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any test-db or qa list collects the executor unittest directory.
fd -t f -e yml -e yaml . tests/integration/test_lists | xargs rg -n 'unittest/executor|unittest\b.*executor'Repository: NVIDIA/TensorRT-LLM
Length of output: 3457
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- target test file ---'
fd -t f 'test_proxy_worker_startup.py' .
printf '%s\n' '--- exact module references ---'
rg -n -F 'unittest/executor/test_proxy_worker_startup.py' tests/integration/test_lists || true
printf '%s\n' '--- executor directory entries ---'
rg -n -F 'unittest/executor' tests/integration/test_lists || true
printf '%s\n' '--- target test functions ---'
target=$(fd -t f 'test_proxy_worker_startup.py' . | head -n 1)
if [ -n "$target" ]; then
rg -n '^def test_|^async def test_' "$target"
fi
printf '%s\n' '--- nearby list syntax ---'
rg -n -C 2 'unittest/(executor|_torch/executor)' tests/integration/test_lists/test-db | head -n 180Repository: NVIDIA/TensorRT-LLM
Length of output: 11788
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- test-list files ---'
git ls-files tests/integration/test_lists | sed -n '1,160p'
printf '%s\n' '--- target path in all test lists ---'
rg -n -F 'unittest/executor/test_proxy_worker_startup.py' tests/integration/test_lists || true
printf '%s\n' '--- target basename in all test lists ---'
rg -n -F 'test_proxy_worker_startup' tests/integration/test_lists || trueRepository: NVIDIA/TensorRT-LLM
Length of output: 4965
Add the new startup tests to a CI test list.
Test coverage summary:
- Added 20 test functions.
- No test functions were modified or removed.
- No
test-db/orqa/list files were modified. - No list entry includes
tests/unittest/executor/test_proxy_worker_startup.py. - Coverage verdict: insufficient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unittest/executor/test_proxy_worker_startup.py` around lines 15 - 39,
Add tests/unittest/executor/test_proxy_worker_startup.py to the appropriate CI
test list or test database entry so all 20 startup test functions execute in CI.
Do not modify or remove the tests; update only the existing list configuration
used for executor unit-test coverage.
Source: Path instructions
|
PR_Github #62990 [ run ] completed with state
|
9ccf505 to
c9b8f19
Compare
|
PR_Github #63197 [ run ] triggered by Bot. Commit: |
|
PR_Github #63197 [ run ] completed with state |
|
PR_Github #63198 [ run ] triggered by Bot. Commit: |
|
PR_Github #63198 [ run ] completed with state |
|
PR_Github #63202 [ run ] triggered by Bot. Commit: |
|
PR_Github #63202 [ run ] completed with state |
|
PR_Github #63204 [ run ] triggered by Bot. Commit: |
|
PR_Github #63204 [ run ] completed with state |
|
PR_Github #63206 [ run ] triggered by Bot. Commit: |
|
PR_Github #63206 [ run ] completed with state |
|
PR_Github #63229 [ run ] triggered by Bot. Commit: |
|
PR_Github #63229 [ run ] completed with state |
|
PR_Github #63230 [ run ] triggered by Bot. Commit: |
|
PR_Github #63230 [ run ] completed with state |
|
PR_Github #63231 [ run ] triggered by Bot. Commit: |
|
PR_Github #63231 [ run ] completed with state |
|
/bot run |
|
PR_Github #63326 [ run ] triggered by Bot. Commit: |
|
PR_Github #63326 [ run ] completed with state
|
The proxy's worker-startup handshake exits only on a leader status
message or on worker death, so a rank that is alive but wedged during
initialization (e.g. inside an NCCL bootstrap collective) leaves the
proxy spinning in worker_init_status_queue.poll(1) with nothing said.
The failure surfaces only as an outer harness timeout, with a
client-side stack that says nothing beyond "waiting on a queue" --
neither which rank is stuck nor whether any rank is stuck at all.
This does not add a deadline. Initialization that is slow but healthy
(a very large checkpoint loading from a cold mount) must not be killed,
so the loop still exits only on ready, init error, or a dead rank. What
changes is that the wait is no longer silent:
* every worker rank arms a watchdog thread before the first collective
and, while its own initialization is still pending, logs its rank/pid
and dumps all of its thread stacks (reusing print_all_stacks) every
TRTLLM_WORKER_INIT_STALL_WARN_SEC seconds (default 600);
* the proxy logs a matching stall report naming the elapsed time and,
where it can see them, how many worker tasks are still running, which
distinguishes a stalled initialization from the already-covered crash.
During init the proxy has an IPC channel to the rank-0 leader only, so a
wedged non-leader is invisible to it: the only process that can say
where rank N is stuck is rank N. Hence the per-rank self-report.
The leader stays armed until it has delivered the ready signal, not
merely until its constructor returns -- that is when the proxy's wait
actually ends, and a leader wedged in between is precisely the case the
proxy cannot see. Subordinates disarm after construction, since they
then block in block_subordinates() for the life of the job.
Attribution is the whole deliverable here, so the report must not claim
more than it knows. RemoteMpiCommSessionClient.submit() returns [], so
under trtllm-llmapi-launch mpi_futures is empty and counting it would
print "0/0 worker task(s) are still running, so no rank has exited" --
a confident liveness statement made with zero visibility, the same
empty-list trap pre_shutdown() already documents. With no futures the
report now says plainly that it cannot tell whether a rank has exited
and names check_worker_error() as the authoritative channel; it does not
call it, because reading it consumes the death notice
_check_remote_worker_death() acts on.
For the same reason the knob is now validated instead of being coerced.
float("nan") does not raise, and every comparison with nan is False, so
a typo'd TRTLLM_WORKER_INIT_STALL_WARN_SEC=nan slipped past the
"period <= 0" disable check and armed a watchdog whose
Event.wait(nan) returns immediately (measured: ~10us, no exception)
rather than sleeping -- a hot loop dumping every thread's stack at
WARNING on every rank. Event.wait(inf) raises OverflowError inside the
watchdog thread instead, silently killing the reporting. Non-finite and
unparsable values now raise, and the proxy reads the knob before any
rank is spawned: startup is exactly where information gets lost, so
swallowing a misconfiguration here is the wrong default.
Both are emitted at WARNING, not ERROR: a slow load is not a fault, and
this fires on that run too. print_all_stacks() grows an optional log
callable so the dump follows the level of the condition that triggered
it; its default stays logger.error for existing callers.
The knob is an environment variable rather than an LlmArgs field
because the TRTLLM-prefixed environment is already forwarded to spawned
MPI ranks, so a single export arms both sides, and the protected LLM API
surface stays untouched.
Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…ivery fails Review catches from @zhaoyangwang-nvidia and CodeRabbit. `notify_with_retry()` returning False means the ready signal never reached the proxy -- the proxy is still blocked in its startup wait, looking for a signal that will never arrive. `worker_init_done.set()` ran regardless, disarming the stall watchdog on the one path where it is most needed and leaving a silent hang: this rank healthy and serving, the proxy waiting forever, nothing reporting either fact. Disarm only on successful delivery. On failure, keep the watchdog armed so the per-rank stall reports keep coming, and log at ERROR rather than WARNING -- this outcome leaves the job hung, which is not a warning-level event. Also annotate `print_all_stacks(log)`, which gained a parameter without types. Tests read the control flow from the AST: reaching this line for real needs a constructed worker, MPI ranks and an engine, but the property under test is which branch sets the event, and that is exactly what the AST shows. One test pins that the success path disarms (otherwise every healthy startup dumps stacks forever) and one that the failure path does not. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run |
c9b8f19 to
3106a0d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
PR_Github #63897 [ run ] triggered by Bot. Commit: |
|
Removed the "ci: full pre-merge approved" label because @JunyiXu-nv could not be verified as an active member of NVIDIA/trt-llm-ci-approvers. Ask a member of that team to apply it. |
|
PR_Github #63897 [ run ] completed with state
|
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The design here is sound — diagnostics without a deadline is the right call given the hardware evidence, and the disarm placement (leader stays armed until the ready signal is delivered, and past a failed delivery) handles the cases that matter. The PR description is a model of what these should look like.
Two things before merge, both flagged inline:
-
The test file never runs in CI.
tests/unittest/executor/files are enrolled per-file intests/integration/test_lists/test-db/l0_*.yml, andtest_proxy_worker_startup.pyisn't in any of them. The AST placement guards you wrote specifically to catch the disarm regression "next time" are inert until the file is enrolled. -
Strict knob parsing is right on the proxy but hazardous on the worker. In mgmn deployments the worker ranks' environment is not the proxy's, and a malformed value raises inside
worker_mainbefore the barrier and before any reporting channel exists — producing exactly the silent multi-rank wedge this PR exists to eliminate.
One non-blocking note: the AST-based tests are brittle to legitimate refactors (renaming worker_init_done, extracting the ready-send into a helper). They fail loudly rather than vacuously, which is the right failure mode, but expect them to need maintenance whenever worker_main is restructured — a comment in worker_main pointing back at the test file would help the next person understand why their refactor broke a test.
| # limitations under the License. | ||
| """Startup handshake between the executor proxy and its MPI workers. | ||
|
|
||
| Every proxy test here drives the real |
There was a problem hiding this comment.
This file isn't enrolled in any test-db list, so none of it runs in pre-merge CI — tests/unittest/executor/ files are listed per-file in tests/integration/test_lists/test-db/l0_*.yml (see test_proxy_fast_death.py in l0_a10.yml:144). Since nothing here needs a GPU, either add it to l0_cpu.yml with pytestmark = pytest.mark.cpu_only (the CPU stage collects with -m cpu_only; without the marker the file is silently deselected), or add it next to test_proxy_fast_death.py in l0_a10.yml. Without enrollment the AST regression guards this file argues for can't catch anything.
| # Arm before the first collective: everything from here to the end of | ||
| # engine construction is init-phase work that can wedge a rank. | ||
| worker_init_done = threading.Event() | ||
| _arm_worker_init_stall_watchdog(worker_init_done) |
There was a problem hiding this comment.
Strict parsing is right on the proxy side, where the ValueError fires before anything is spawned — but here it fires inside a rank, before the barrier below and before worker_init_status_queue exists, so there is no channel to report through. In the local-spawn case the proxy's pre-spawn check shields this (env is forwarded), but under trtllm-llmapi-launch the workers' environment is independent of the proxy's: a malformed value there kills one rank unreported, the remaining ranks wedge in mpi_comm().barrier(), and the proxy has no futures to observe (RemoteMpiCommSessionClient.submit() returns []) — a silent hang of exactly the class this PR removes, with no watchdog armed to report it. Suggest catching ValueError inside _arm_worker_init_stall_watchdog only (the proxy calls worker_init_stall_warn_sec() directly, so it stays strict): log at ERROR and fall back to the default period, so a rank-side misconfiguration degrades to a noisy default instead of an invisible wedge.
4 participants
The gap
The proxy's worker-startup handshake (
proxy.py:645-654) exits on exactly two conditions: the leader sends a status message, or a worker task completes (i.e. a rank died). A rank that is alive but wedged during initialization — typically inside a bootstrap collective — satisfies neither, so the proxy spins inworker_init_status_queue.poll(1)indefinitely.This is a real sighting, not a hypothesis. Recovered from a
pytest-timeoutstack dump:proxy.py:646→ipc.py:181 zmq_poll, after 60.2 minutes of silence, ended by the harness's--timeout=3600. The client-side stack said nothing beyond "waiting on a queue" — not which rank was stuck, nor whether any rank was stuck at all.Nothing existing covers it. The
HangDetectorarms only insidePyExecutor's loops, andPyExecutorhas not been constructed yet. The proxy's fast-death path keys onfut.done(), and the worker is alive.What this does — and does not — do
It adds no timeout and it kills nothing. The loop still exits only on ready, init error, or a dead rank. Initialization that is slow but healthy (a large checkpoint from a cold mount) must not be killed.
What changes is that the wait is no longer silent:
TRTLLM_WORKER_INIT_STALL_WARN_SECseconds (default 600);During init the proxy has an IPC channel to the rank-0 leader only, so a wedged non-leader is invisible to it. The only process that can say where rank N is stuck is rank N; hence the per-rank self-report.
A bound was designed, implemented, and then removed. An opt-in
TRTLLM_WORKER_INIT_TIMEOUT_SECexisted until hardware testing showed it could not deliver on its promise (see "Out of scope" below). Shipping it would have meant offering a knob that logs "giving up" next to a process still alive and still holding GPU memory. That history is recorded here deliberately.Hardware evidence (2xH100 PCIe, Qwen3-0.6B, tp=2)
(a) Arm and disarm across a healthy startup (
TRTLLM_WORKER_INIT_STALL_WARN_SEC=1): both ranks reported 10 times during init; zero watchdog lines in the 142 log lines afterLLM CONSTRUCTED, spanning a post-init sleep, a served request and a post-request sleep.PY_RC=0.(b) A genuinely wedged rank —
SIGSTOPsent to rank 1 mid-init:One rank names where it is stuck; the other is identified by rank and pid as the one that went silent. That attribution is the entire justification for this change.
(c) Slow but healthy. An unrelated run on a cold JIT cache took over 15 minutes to construct. It reported throughout at WARNING, correctly named the straggler, and was not killed. This is the case a deadline would have destroyed.
Out of scope: a pre-existing teardown weakness
Measured while the bound still existed, and reported because it is useful, not because this PR fixes it. With
TRTLLM_WORKER_INIT_TIMEOUT_SEC=60against aSIGSTOP-ed rank, the give-up fired on time at 60s — butMpiPoolSession.shutdown(wait=True)never returned andshutdown_abort's 60sMPI_Abortescalation never logged at all. At t=600s both ranks were still alive holding 2373 MiB + 2335 MiB; the process required an external kill.This is pre-existing in
shutdown_abort, already reachable from the "worker returned error" path, and no longer reachable from this change now that the bound is gone.Known residual
If the leader raises inside
with worker:before reaching the post-ready disarm, its watchdog keeps logging until the process exits. Daemon thread, noise only.Testing
15 unit tests, no GPU required, driving the real
GenerationExecutorProxy._start_executor_workersand the real watchdog helpers. Ten-mutant campaign, all ten killed, including the leader-disarm regression that hardware originally exposed (the watchdog was disarmed after construction rather than after the ready signal — which would have left a wedged leader silent from every rank while every unit test passed).The leader/subordinate disarm placement is pinned structurally over
worker_main's AST, because behavioural coverage there needs a live MPI world; the assertions carry the invariant in their failure messages.Dev Engineer Review
TRTLLM_WORKER_INIT_STALL_WARN_SEC.print_all_stacksaccepts an optional logging callable and preserves the default logger behavior.ValueErrorbefore worker submission.check_worker_error(). Reports identify when rank liveness cannot be determined.QA Engineer Review
tests/unittest/executor/test_proxy_worker_startup.py.tests/integration/test_lists/.test-db/orqa/coverage entry is listed.